Skip to content

Conversation

ImadRedwan
Copy link

@ImadRedwan ImadRedwan commented Jul 19, 2025

Motivation

Currently, parse_param_general handles both the general function parameters and the optional parsing of the self receiver as the first parameter.

This mixes two distinct responsibilities:

  • Parsing the self receiver (which has unique grammar and semantics),
  • Parsing general parameters (typed, optionally named).

This makes the function harder to understand and harder to test.


What This PR Does

  • Moves the parsing of the self parameter out of parse_param_general.
  • Handles it explicitly in parse_fn_params before entering the general parameters loop.
  • Removes the first_param boolean and its branching logic.
  • Keeps req_name as-is (required name enforcement still makes sense).

Behavior After Refactor

Behavior is functionally identical. The only change is structural:


Tests

  • Existing parser tests still pass.
  • Closure syntax and self receiver handling remain correct.
  • No behavioral changes were introduced.

Benefits

  • Cleaner separation of concerns: self parsing is specialized and now handled separately.
  • Improved readability: General parameters no longer have to reason about self.

Also parsing the self parameter can be extracted into a small inline function to isolate its logic from regular parameter parsing.

Reviewer Notes

  • Feel free to suggest if self parsing should be refactored further.
  • This cleanup was kept deliberately minimal to avoid breaking existing logic or tests.

Moves responsibility for parsing `self` out of `parse_param_general` and into `parse_fn_params`. This simplifies the function by reducing branching and
clearly separating the parsing of `self` from regular parameters.

This is a non-functional change; behavior is preserved.
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 19, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
             return Ok(ThinVec::new());
         }
 
-
         // Possibly parse `self`. Recover if we parsed it and it wasn't allowed here.
         let self_attrs = self.parse_outer_attributes()?;
         let self_param = if let Some(mut param) = this.parse_self_param()? {
Diff in /checkout/compiler/rustc_parse/src/parser/item.rs:2963:
             param.attrs = self_attrs;
             param
         };
-        
 
         let (mut params, _) = self.parse_paren_comma_seq(|p| {
             p.recover_vcs_conflict_marker();
Diff in /checkout/compiler/rustc_parse/src/parser/item.rs:2984:
                 // Create a placeholder argument for proper arg count (issue #34264).
                 Ok(dummy_arg(Ident::new(sym::dummy, lo.to(p.prev_token.span)), guar))
             });
-            
+
             param
         })?;

@ImadRedwan ImadRedwan closed this Jul 19, 2025
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 19, 2025
@ImadRedwan ImadRedwan deleted the patch-1 branch July 19, 2025 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants